Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
russom-g
added
the
Needs Review
russom-g
changed the title
hackertainment
added
Review in progress
hackertainment
left a comment
hackertainment
left a comment
There was a problem hiding this comment.
Thank you for your coursework. Keep it up.
| test("should count multiple occurrences of a character", () => { | ||
| const str = "aaaaa"; | ||
| const char = "a"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(5); | ||
| }); |
There was a problem hiding this comment.
This is a special case where char appears in the whole string str. Please add a general test case. Thank you.
There was a problem hiding this comment.
I have added another test case.
| test("should return 0 for a character with no occurrences", () => { | ||
| const str = "aaaaa"; | ||
| const char = "b"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
This test case is correct. However, there should be a special str which gives 0 anyway. Can you think of it? If so, please add it as well. Thank you.
There was a problem hiding this comment.
Would that be an empty string("") ?
There was a problem hiding this comment.
Good try and almost correct. However, your original test case of counting char = "b" in str = "aaaaa" should still be kept. For the empty string test, it should be counting char = "b" in empty string str = "", but not counting empty character char = "" in str = "aaaaa" as one could say there are 6 (or even infinity) of empty occurrences between the 5 as. So char = "" is not required in this exercise but str = "" would be needed.
| test("should append 'th' for numbers ending with 11, 12 and 13", () => { | ||
| expect(getOrdinalNumber(11)).toEqual("11th"); | ||
| expect(getOrdinalNumber(12)).toEqual("12th"); | ||
| expect(getOrdinalNumber(13)).toEqual("13th"); | ||
| }); |
There was a problem hiding this comment.
These are numbers exactly equal to 11/12/13, how about larger numbers ended with 11/12/13 ? Please add those tests as well. Thank you.
There was a problem hiding this comment.
Your test cases are correct. Just for you to think a little bit more, besides const str = "hello", what special string should also be added to each of the test cases?
There was a problem hiding this comment.
An empty string? Additional test with empty string input is added.
Thanks for your support with the review and feedback.
There was a problem hiding this comment.
That's right. Also, it would be nice to test empty string with negative count to see whether it correctly throws error or incorrectly returns 0. Since your program logic is correct, I will let you pass for this one and just have a better test cases coverage in the future. Thank you.
russom-g
added
Needs Review
|
You are almost there and just remain this new comment #1723 (comment) . Please fix it and this PR will be good to be finished. Thank you. |
hackertainment
added
Reviewed
Learners, PR Template
Self checklist
Task code
CYF-1060
Changelist
Wrote tests with Jest to me the criteria first. Then implemented the function. Finally tests run to check functions meet the requirements.